Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mergify: enable for any pull request #12274

Merged
merged 3 commits into from
Jan 20, 2020
Merged

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Jan 17, 2020

Summary of changes

Add mergify bot to all PRs (all branches). The configuration already resides on master but not yet enabled, only been enabled for feature-mergify to test it.

Docs: https://doc.mergify.io/

The configuration should follow our workflow. If it does not, we should fix via new pull request anytime (note: one drawback, the config is fetched from the default branch, in our case master. So even rules for non master, need to be in the config there). The auto merge is disabled, the bot does not have write access to any of the branches anyway. This bot can move labels, post comments.

To verify for each PR, use "Checks" where you can review Mergify conditions.

To disable, just revert this comment - to have all rules only on feature-mergify for instance. Hopefully, this won't be necessary and we tune the rules once we get more PRs involved (compare to few testing we did).

Impact of changes

Migration actions required

Documentation


Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[X] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@bulislaw

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 17, 2020

Notes:

  • reviewers count (request review or approvals) need write access (how Github defines this and mergify uses), therefore request for review or approved counts are only valid for write access members. We will fix this in the following days.
    Currently, tech leads and maintainers reviews count, so should work in most cases but will be improved
  • default branch config is used always, so even this PR changes to the conflict won't affect the current job (as default branch config is fetched). This is one limitation to be aware of (rules can be applied for specific branches, mergify can rescan PRs so once master is updated). It was requested to be added in mergify-engine issue tracker.
  • automerge is not enabled, instead "ready for merge" label once PR is ready. We will still manually merge PRs for the time being. There are couple of other things that should be addressed before we can go to "automerge".

This should minimize the time between workflow stages. If you find an error, please review "Checks" - Mergify rules and let us know what is not working, or just fix it based on the Mergify documentation.

This PR won't have mergify bot active (see 2nd note). Once merged, it will start.

@ciarmcom ciarmcom requested review from bulislaw and a team January 17, 2020 14:00
@ciarmcom
Copy link
Member

@0xc0170, thank you for your changes.
@bulislaw @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a state:
In 'needs work' and new commits are made --> needs review ?

@@ -53,7 +51,6 @@ pull_request_rules:
# From needs: review to needs: work - CI failure
- name: "label needs: work when Jenkins CI failed - pr head"
conditions:
- base~=feature-mergify
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also have a needs: ci label here that may need to be removed if travis was restarted during a CI cycle ?

@@ -66,7 +63,6 @@ pull_request_rules:
# From needs: review to needs: work - CI failure
- name: "label needs: work when Jenkins CI failed - any of the pipeline"
conditions:
- base~=feature-mergify
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, may need to also remove needs: ci if present

Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor changes needed

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 20, 2020

I added CI label removal.

In 'needs work' and new commits are made --> needs review ?

There is no condition for this at the moment, will check and request this one.
What we can enable is on github, to request new review once PR is updated.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 20, 2020

There is this condition: https://doc.mergify.io/examples.html#removing-stale-reviews (not yet complete, see below).

there's an issue about similar functionality we want: Mergifyio/mergify#360. I've left there a comment about what can be done.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 20, 2020

I updated stale reviews update. I added this:

    conditions: []
    actions:
      dismiss_reviews:
        approved: True
        changes_requested: True

Means after the branch is updated, it will dismissed approved/requested changes and send a message about the update.

Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adbridge
Copy link
Contributor

CI started

@0xc0170 0xc0170 added ready for merge release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 and removed needs: CI labels Jan 20, 2020
@adbridge adbridge merged commit a81f016 into ARMmbed:master Jan 20, 2020
@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 20, 2020

One additional stage there - thinking about the updating PR after needs: work state. As we have jenkins pr-head there and its yellow whenever PR is created or updated. It means updated PR is when pr-head is yellow (no success or failure) and PR is in "needs: work" state. Action would be to move from needs work to needs review.

This should go along with dismissed reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants